Fix upgrading indices which use a custom similarity plugin. #26985

xabbu42 · 2017-10-12T12:52:02Z

Use a fake similarity map that always returns a value in MetaDataIndexUpgradeService.checkMappingsCompatibility instead of an empty map.

Closes #25350

elasticmachine · 2017-10-12T12:52:03Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

rjernst

Looks good. I have two small requests.

rjernst · 2017-10-12T20:41:54Z

core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexUpgradeService.java

+
+                @Override
+                public Set<Entry<String, SimilarityProvider.Factory>> entrySet() {
+                    return Collections.emptySet();


I realize the analyzer hack does this, but it is inconsistent with what is returned above. I would rather throw a UEO here so that if entrySet begins to be used, we will catch this rather than get weird behavior. (And we should, in a followup or in this PR, fix the other entrySet() impl as well).

I can do that but need to know what an UEO is (I'm not a Java programmer).

@xabbu42 I think that @rjernst meant "UOE" as in UnsupportedOperationException.

Yes, Jason is correct on what I meant.

I get test failures if make this change. For example:

Suite: org.elasticsearch.cluster.metadata.MetaDataIndexUpgradeServiceTests 1> [2017-10-14T16:43:29,230][INFO ][o.e.c.m.MetaDataIndexUpgradeServiceTests] [testPluginUpgradeFailure]: before test 1> [2017-10-14T16:43:29,233][INFO ][o.e.c.m.MetaDataIndexUpgradeServiceTests] [testPluginUpgradeFailure]: after test 2> REPRODUCE WITH: gradle :core:test -Dtests.seed=67F6816A4DF09ADB -Dtests.class=org.elasticsearch.cluster.metadata.MetaDataIndexUpgradeServiceTests -Dtests.method="testPluginUpgradeFailure" -Dtests.security.manager=true -Dtests.locale=sr -Dtests.timezone=Asia/Katmandu FAILURE 0.03s | MetaDataIndexUpgradeServiceTests.testPluginUpgradeFailure <<< FAILURES! > Throwable #1: org.junit.ComparisonFailure: expected:<[unable to upgrade the mappings for the index [[foo/BOOM]]]> but was:<[Cannot upgrade index foo]> > at __randomizedtesting.SeedInfo.seed([67F6816A4DF09ADB:37D52F4B7D45621E]:0) > at org.elasticsearch.cluster.metadata.MetaDataIndexUpgradeServiceTests.testPluginUpgradeFailure(MetaDataIndexUpgradeServiceTests.java:141) > at java.lang.Thread.run(Thread.java:748) 1> [2017-10-14T16:43:29,258][INFO ][o.e.c.m.MetaDataIndexUpgradeServiceTests] [testPluginUpgrade]: before test 1> [2017-10-14T16:43:29,263][INFO ][o.e.c.m.MetaDataIndexUpgradeServiceTests] [testPluginUpgrade]: after test

I think this happens because IndexAnalyzers iterates over the analyzers/normalizers when in close(). Go ahead and add back the empty map, but please add a comment noting the reason?

rjernst · 2017-10-12T20:52:30Z

core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexUpgradeService.java

@@ -136,7 +137,24 @@ private void checkMappingsCompatibility(IndexMetaData indexMetaData) {
            // We cannot instantiate real analysis server at this point because the node might not have
            // been started yet. However, we don't really need real analyzers at this stage - so we can fake it
            IndexSettings indexSettings = new IndexSettings(indexMetaData, this.settings);
-            SimilarityService similarityService = new SimilarityService(indexSettings, null, Collections.emptyMap());
+            final Map<String, SimilarityProvider.Factory> similarityMap = new AbstractMap<String, SimilarityProvider.Factory>() {


I would add a comment here that having a dummy map works because we assume any similarity providers were known before the upgrade, so allowing any key below is ok.

I did test the patch with a elasticsearch server without the custom similarity installed. The server starts but does not recover the index with an appropriate error message. I did not test restoring a snapshot from an old version. But as far as I understand the situation, we do not assume all used similarities are actually known here, but just defer the error to a point when we actually know all available similarities.

Defer is what I meant by "assume". We allow any similarity name to be looked up without error. We don't actually use it, we just need to ensure the lookup does not fail. This is ok because we assume whatever was present when ES last shut down had all known similarities.

xabbu42 · 2017-10-30T12:49:31Z

Just to be clear, I'm waiting on further input here for both the requested changes.

rjernst · 2017-11-10T19:24:08Z

@xabbu42 Sorry for the delayed response. Could you please update this PR as per the new comments?

xabbu42 · 2017-11-11T11:59:28Z

@rjernst no problem, I just wanted to make sure nobody is waiting on me. I hope the updated PR is ok now.

rjernst · 2017-11-27T22:27:22Z

@elasticmachine test this please

rjernst · 2017-11-28T20:56:56Z

@xabbu42 Can you fix the tabs? They should be spaces:

23:25:22 Execution failed for task ':core:forbiddenPatterns'.
23:25:22 > Found invalid patterns:
23:25:22   - tab on line 159 of core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexUpgradeService.java
23:25:22   - tab on line 160 of core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexUpgradeService.java
23:25:22   - tab on line 181 of core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexUpgradeService.java
23:25:22   - tab on line 182 of core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexUpgradeService.java

xabbu42 · 2017-11-29T13:10:05Z

Ups sorry about that, its fixed now.

rjernst · 2017-12-05T23:06:19Z

@elasticmachine test this please

rjernst · 2017-12-21T20:47:47Z

@elasticmachine test this please

elasticmachine · 2017-12-22T19:32:15Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

rjernst · 2017-12-22T19:33:25Z

@elasticmachine test this please

rjernst · 2017-12-22T21:09:05Z

@elasticmachine test this please

Use a fake similarity map that always returns a value in MetaDataIndexUpgradeService.checkMappingsCompatibility instead of an empty map. Closes #25350

rjernst · 2018-01-09T01:27:38Z

Thanks @xabbu42

* master: Use Gradle wrapper when building BWC Painless: Add a simple cache for whitelist methods and fields. (elastic#28142) Fix upgrading indices which use a custom similarity plugin. (elastic#26985) Fix Licenses values for CDDL and Custom URL (elastic#27999) Cleanup TcpChannelFactory and remove classes (elastic#28102) Fix expected plugins test for transport-nio [Docs] Fix Date Math example descriptions (elastic#28125) Fail rollover if duplicated alias found in template (elastic#28110) Avoid concurrent snapshot finalizations when deleting an INIT snapshot (elastic#28078) Deprecate `isShardsAcked()` in favour of `isShardsAcknowledged()` (elastic#27819) [TEST] Wait for replicas to be allocated before shrinking Use the underlying connection version for CCS connections (elastic#28093) test: do not use asn fields Test: Add assumeFalse for test that cannot pass on windows Clarify reproduce info on Windows Remove out-of-date projectile file

* master: (27 commits) Declare empty package dirs as output dirs Consistent updates of IndexShardSnapshotStatus (elastic#28130) Fix Gradle wrapper usage on Windows when building BWC (elastic#28146) [Docs] Fix some typos in comments (elastic#28098) Use Gradle wrapper when building BWC Painless: Add a simple cache for whitelist methods and fields. (elastic#28142) Fix upgrading indices which use a custom similarity plugin. (elastic#26985) Fix Licenses values for CDDL and Custom URL (elastic#27999) Cleanup TcpChannelFactory and remove classes (elastic#28102) Fix expected plugins test for transport-nio [Docs] Fix Date Math example descriptions (elastic#28125) Fail rollover if duplicated alias found in template (elastic#28110) Avoid concurrent snapshot finalizations when deleting an INIT snapshot (elastic#28078) Deprecate `isShardsAcked()` in favour of `isShardsAcknowledged()` (elastic#27819) [TEST] Wait for replicas to be allocated before shrinking Use the underlying connection version for CCS connections (elastic#28093) test: do not use asn fields Test: Add assumeFalse for test that cannot pass on windows Clarify reproduce info on Windows Remove out-of-date projectile file ...

Use a fake similarity map that always returns a value in MetaDataIndexUpgradeService.checkMappingsCompatibility instead of an empty map. Closes #25350 relates #26985

Fix upgrading indices which use a custom similarity plugin.

824e545

rjernst requested changes Oct 12, 2017

View reviewed changes

add/combine comments for the fake similarity map

976b520

cbuescher added the :Core/Infra/Plugins Plugin API and infrastructure label Nov 16, 2017

Fix wrong tabs

aa4be49

Merge branch 'master' into review/26985

28309da

rjernst approved these changes Dec 22, 2017

View reviewed changes

rjernst merged commit 06d1ed8 into elastic:master Jan 9, 2018

clintongormley added >bug v7.0.0 v6.3.0 labels Jan 15, 2018

rjernst added v6.2.0 and removed v6.3.0 labels Jan 15, 2018

thomas11 mentioned this pull request Feb 22, 2018

Backport PR 26985, upgrading indices with a custom similarity plugin #28795

Merged

rjernst added the v5.6.9 label Mar 14, 2018

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix upgrading indices which use a custom similarity plugin. #26985

Fix upgrading indices which use a custom similarity plugin. #26985

xabbu42 commented Oct 12, 2017

elasticmachine commented Oct 12, 2017

rjernst left a comment

rjernst Oct 12, 2017

xabbu42 Oct 13, 2017

jasontedor Oct 13, 2017 •

edited

Loading

rjernst Oct 13, 2017

xabbu42 Oct 14, 2017 •

edited

Loading

rjernst Nov 10, 2017

rjernst Oct 12, 2017

xabbu42 Oct 13, 2017

rjernst Nov 10, 2017

xabbu42 commented Oct 30, 2017

rjernst commented Nov 10, 2017

xabbu42 commented Nov 11, 2017

rjernst commented Nov 27, 2017

rjernst commented Nov 28, 2017

xabbu42 commented Nov 29, 2017

rjernst commented Dec 5, 2017

rjernst commented Dec 21, 2017

elasticmachine commented Dec 22, 2017

rjernst commented Dec 22, 2017

rjernst commented Dec 22, 2017

rjernst commented Jan 9, 2018

Fix upgrading indices which use a custom similarity plugin. #26985

Fix upgrading indices which use a custom similarity plugin. #26985

Conversation

xabbu42 commented Oct 12, 2017

elasticmachine commented Oct 12, 2017

rjernst left a comment

Choose a reason for hiding this comment

rjernst Oct 12, 2017

Choose a reason for hiding this comment

xabbu42 Oct 13, 2017

Choose a reason for hiding this comment

jasontedor Oct 13, 2017 • edited Loading

Choose a reason for hiding this comment

rjernst Oct 13, 2017

Choose a reason for hiding this comment

xabbu42 Oct 14, 2017 • edited Loading

Choose a reason for hiding this comment

rjernst Nov 10, 2017

Choose a reason for hiding this comment

rjernst Oct 12, 2017

Choose a reason for hiding this comment

xabbu42 Oct 13, 2017

Choose a reason for hiding this comment

rjernst Nov 10, 2017

Choose a reason for hiding this comment

xabbu42 commented Oct 30, 2017

rjernst commented Nov 10, 2017

xabbu42 commented Nov 11, 2017

rjernst commented Nov 27, 2017

rjernst commented Nov 28, 2017

xabbu42 commented Nov 29, 2017

rjernst commented Dec 5, 2017

rjernst commented Dec 21, 2017

elasticmachine commented Dec 22, 2017

rjernst commented Dec 22, 2017

rjernst commented Dec 22, 2017

rjernst commented Jan 9, 2018

jasontedor Oct 13, 2017 •

edited

Loading

xabbu42 Oct 14, 2017 •

edited

Loading